Skip to content

fix(core): compare CUdeviceptr as int so the VMM grow fast path can run#2407

Open
aryanputta wants to merge 1 commit into
NVIDIA:mainfrom
aryanputta:fix/vmm-fast-path-ptr-compare
Open

fix(core): compare CUdeviceptr as int so the VMM grow fast path can run#2407
aryanputta wants to merge 1 commit into
NVIDIA:mainfrom
aryanputta:fix/vmm-fast-path-ptr-compare

Conversation

@aryanputta

@aryanputta aryanputta commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Description

Addresses defect 2 of #2388 (the dead grow fast path). The other three defects in that issue are not touched here, so this does not close it.

modify_allocation() asks cuMemAddressReserve for the address immediately after the current range, then checks whether the driver granted that exact address before taking the fast path:

res, new_ptr = driver.cuMemAddressReserve(..., int(buf.handle) + aligned_prev_size, 0)

if res != driver.CUresult.CUDA_SUCCESS or new_ptr != (int(buf.handle) + aligned_prev_size):

new_ptr is a CUdeviceptr. That type defines __int__ and __repr__ but no __eq__ or __richcmp__ (cuda_bindings/cuda/bindings/driver.pyx), so comparing it against a plain int falls back to identity and is unconditionally unequal. The guard is therefore always taken, _grow_allocation_fast_path is unreachable, and every grow runs the slow path: a full re-reserve plus remap, with the buffer's base pointer changing even when the driver did grant a contiguous extension.

The fix is the int() conversion suggested in the issue.

Behavior change, not a pure cleanup

This makes a previously unreachable path live. After it, a grow the driver can satisfy contiguously preserves the base pointer instead of moving it. That is the documented intent of the fast path, but it has never actually executed, so it deserves explicit attention in review. Added to the 1.2.0 release notes for that reason. The existing test asserting a grown buffer's pointer may change (test_vmm_allocator_grow_allocation) still passes, since it only requires a non-null handle.

Since the fast path becomes reachable for the first time, I checked the one line in it that looked risky. buf._size = new_size still carries a TODO: #2049 This is a real bug, accessing _size which doesn't exist. That TODO is stale on the Cython side: _size was moved into a cdef public block in _buffer.pxd by #2216 and #2049 is closed, so the assignment works on a real Buffer. The # type: ignore[attr-defined] is still required because _buffer.pyi does not expose _size, so I left both the ignore and the TODO alone rather than widening this PR. Happy to clean that up separately if you want it.

Testing

test_vmm_allocator_grow_dispatches_to_fast_path stubs cuMemAddressReserve to grant exactly the address that was requested and asserts the dispatch reaches the fast path. It should fail on current main, recording "slow". It follows the stubbing style of the adjacent test_vmm_allocator_grow_allocation_fast_path.

That existing test covered _grow_allocation_fast_path by calling it directly, which is why the broken dispatch was invisible: it stays green whether or not modify_allocation can actually reach the helper. The gap was coverage of the decision that reaches it.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Jul 22, 2026
@aryanputta
aryanputta marked this pull request as draft July 22, 2026 15:38
@aryanputta
aryanputta marked this pull request as ready for review July 24, 2026 00:30
@aryanputta

aryanputta commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

I have been assigned #2388, so I am taking this out of draft. It still covers defect 2 only and does not close the issue; I can follow up on defects 1, 3, and 4 in separate PRs if that is the split you prefer.

One thing for a reviewer: workflows on this PR are still waiting on vetter validation, and I do not have an NVIDIA GPU, so test_vmm_allocator_grow_dispatches_to_fast_path has not run anywhere yet. It is expected to fail on current main by recording "slow".

@aryanputta
aryanputta force-pushed the fix/vmm-fast-path-ptr-compare branch from 513ebfc to 57a7a0b Compare July 24, 2026 00:48
@aryanputta

aryanputta commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Current head is 24f3f6c, rebased onto main. Changes since I opened this:

  • Dropped the explanatory comment above the guard. It described why the old comparison was wrong, which belongs in the PR description rather than in the source.
  • Dropped test_cudeviceptr_is_never_equal_to_plain_int. It only asserted that a bindings type has no __eq__, which is verifiable by reading driver.pyx, so it was not earning its place in the suite.
  • Added a bullet to the cuda.core 1.2.0 release notes under "Fixes and enhancements", since this changes observable behavior of a stable API rather than being an internal cleanup.
  • The remaining test is unchanged in substance and the one-line int() fix is unchanged.

The PR description also records something I checked while reviewing this: the TODO: #2049 above buf._size = new_size in _grow_allocation_fast_path is stale, since _size became cdef public in #2216 and #2049 is closed. I left the TODO and the type: ignore in place because _buffer.pyi still does not expose _size, but it is worth knowing that the assignment does work on a real Buffer, given this PR is what makes that line reachable.

`modify_allocation()` asks `cuMemAddressReserve` for the address immediately
after the current range, then checks whether the driver granted that exact
address before taking the fast path.

`cuMemAddressReserve` returns `new_ptr` as a `CUdeviceptr`. That type defines
`__int__` and `__repr__` but no `__eq__` or `__richcmp__`, so `new_ptr != <int>`
falls back to identity comparison and is unconditionally true. The guard is
therefore always taken and `_grow_allocation_fast_path` is unreachable: every
grow runs the slow path, re-reserving the whole range and remapping it, so the
buffer's base pointer changes even when a contiguous extension was available.

Convert with `int()` before comparing. Reported as defect 2 in NVIDIA#2388.

`test_vmm_allocator_grow_dispatches_to_fast_path` stubs `cuMemAddressReserve`
to grant the requested address and asserts the dispatch reaches the fast path.
It fails on the current code, which records "slow".
`test_vmm_allocator_grow_allocation_fast_path` already exercised
`_grow_allocation_fast_path` directly, so the helper itself is covered; what was
missing was coverage of the dispatch decision that reaches it.

This makes a previously unreachable code path live, so it is a behavior change
rather than a pure cleanup: successful contiguous grows now preserve the base
pointer instead of moving it. Noted in the 1.2.0 release notes.

Signed-off-by: Aryan Putta <aryansputta@gmail.com>
@aryanputta
aryanputta force-pushed the fix/vmm-fast-path-ptr-compare branch from 57a7a0b to 24f3f6c Compare July 24, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.core Everything related to the cuda.core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant